Addendum: Deep Handoff Persistence Findings

After deeper analysis of the orchestrator integration path (lib.rs handoff flow), three additional major findings:

M1: Non-atomic three-step handoff in AgentOrchestrator::handoff()

The handoff() method performs three sequential side effects:

1. Write .handoff-{agent}.json to disk
2. Insert into in-memory HandoffBuffer
3. Append to JSONL HandoffLedger

If step 3 fails (disk full, permissions), the file and buffer entry persist without an audit trail. The ledger is the audit log and should be the commit point -- write it first, then the file and buffer.

M2: write_to_file used instead of write_to_file_atomic

The orchestrator calls ctx.write_to_file() (non-atomic) for the per-agent handoff file. If the process crashes mid-write, the target agent reads truncated JSON. The atomic variant (write_to_file_atomic) already exists but isn't used here.

M3: Predictable temp file name in write_to_file_atomic

handoff.rs:114 -- temp path is .tmp.{filename} with no random component. Concurrent atomic writes to the same target race on the same temp path. Consider using tempfile::NamedTempFile::persist() or adding a PID/random suffix.

Minor additions

• to_json() uses to_string_pretty (multi-line output) -- confusing when used near JSONL contexts. Consider renaming to to_json_pretty().
• No remove() method on HandoffBuffer -- consumed handoffs can only expire via TTL, not be explicitly cleared.
• No test for read_all() on corrupt JSONL lines or non-existent files.

Addendum: Deep Budget Gates & Guard Findings

Critical/Major

C1: TOCTOU in record_cost -- spend-then-check is not atomic (cost_tracker.rs:58-62)

fetch_add returns the previous value, but the code discards it and calls check() which does a separate load. Two concurrent record_cost calls can both exceed the budget but both see WithinBudget. Fix: compute the verdict from fetch_add's return value + sub_cents directly, eliminating the second load.

C2: Negative/NaN cost_usd silently produces wrong results (cost_tracker.rs:59)

(cost_usd * SUB_CENTS_PER_USD as f64).round() as u64 -- negative values saturate to 0, NaN produces garbage. A cost-tracking system should reject non-finite and negative values explicitly.

M1: Guard substring patterns too broad (guard_suspicious.json)

• "ssh" matches cat ~/.ssh/config, ls .ssh/, filenames containing "ssh"
• "nc" matches once, ounce, encapsulate
• These need trailing space or word-boundary awareness to avoid false positives

M2: git branch -d (safe delete) blocked alongside -D (force delete) (guard_destructive.json:109-113)

The pattern "git branch -d" matches the safe variant that refuses to delete unmerged branches. Only git branch -D should be in the destructive list.

M3: chmod blocked entirely is overly aggressive (guard_destructive.json:54-58)

Agents cannot chmod +x their own build scripts. This should be Sandbox tier, not Block, or have allowlist entries for safe paths (e.g., within working directory).

M4: CostSnapshot::verdict is a format!("{:?}") String (cost_tracker.rs:115)

Produces unstable debug output like "NearExhaustion { spent_cents: 8100, budget_cents: 10000 }". Consumers parsing this will break. BudgetVerdict should derive Serialize and be used directly.

Missing coverage

• No concurrency tests for CostTracker despite using AtomicU64
• No tests for NaN/Infinity/negative cost inputs
• No tracing spans in either cost_tracker or guard_patterns -- audit logging is important for security and financial systems
• Guard does not handle shell metacharacter evasion (git\ reset\ --hard, double spaces, backtick expansion) -- known limitation of substring matching, worth documenting

Addendum: Deep Compound Review Swarm Findings

Critical

C1: Timed-out child processes are not killed (compound.rs:408-424)

When tokio::time::timeout fires on cmd.output(), the child process continues running as an orphan. tokio::process::Command::output() owns the Child internally; dropping the future does NOT kill the child. Repeated review cycles can accumulate zombie LLM processes consuming memory and API credits. Fix: use Command::spawn(), hold the Child handle, and child.kill().await in the timeout branch.

C2: No cli_tool allowlist validation (compound.rs:385)

cli_tool is passed directly to Command::new(). Today it's hardcoded in default_groups(), but SwarmConfig and ReviewGroupDef are public with pub fields. If config ever comes from a file (already Deserialize), this is command injection. Validate against an allowlist (["opencode", "claude", "codex"]).

Major

M1: extract_review_output markdown code block parsing broken for multi-line JSON (compound.rs:448-464)

Parser iterates line-by-line, so a JSON block spanning multiple lines inside ```json ... ``` fences will never match. Real LLM output almost always spans multiple lines. The test passes only because the fixture has everything on one line. Fix: accumulate lines between fence delimiters before parsing.

M2: Session auto-approves all command execution (session.rs:424-433)

handle_server_request returns {"approved": true} unconditionally for both command execution and file change approvals. In a multi-agent swarm, any agent can execute arbitrary shell commands and modify any file without oversight. The ScopeRegistry is never consulted here.

M3: ScopeRegistry is built but not wired into compound review

The scope reservation system exists but CompoundReviewWorkflow::run() never creates or uses a ScopeRegistry. All agents share the same worktree and full changed files list. Concurrent review runs could trample each other.

M4: Git flag injection possible (compound.rs:323-337)

git_ref and base_ref passed directly as args to git diff without -- separator. A ref like --output=/etc/passwd would be interpreted as a flag. Command prevents shell injection but not flag injection.

M5: Worktree cleanup races with running agents (compound.rs:261-264)

Worktree removal happens immediately after collection deadline. If deadline fired, spawned agents may still be running with current_dir pointing at the now-deleted worktree.

Missing

• AdfEnvelope types exist but are never constructed/emitted by the compound workflow -- incomplete integration
• Token/cost tracking not wired in compound path (raw Command::output() discards cost data)
• No retry logic for transient agent failures
• No circuit breaker or rate limiter for concurrent LLM CLI calls

Addendum: Deep Self-Learning Findings

Critical

C1: procedure module is #[cfg(test)] only -- entire ProcedureStore is dead code in release builds (learnings/mod.rs:30)

#[cfg(test)]
mod procedure;

The ProcedureStore and all its methods are gated behind #[cfg(test)]. This means the procedure storage system is completely unavailable in release builds. Given that doc examples reference terraphim_agent::learnings::procedure::ProcedureStore, this is almost certainly a bug, not intentional.

C2: TOCTOU race on ProcedureStore file operations (procedure.rs:91-110, 210-225)

Every mutating method (save, save_with_dedup, update_confidence, delete) does load-all, mutate in memory, truncate, rewrite. No file locking (flock, advisory lock, or atomic rename). Concurrent calls silently clobber each other's writes. Relevant because the hook system (post_tool_use.sh) can trigger concurrent writes.

Major

M1: McpToolIndex search clones thesaurus N times (mcp_tool_index.rs:149)

find_matches(&search_text, thesaurus.clone(), false) is called inside a loop over all tools. The Aho-Corasick automaton is rebuilt from the thesaurus clone for each tool. Should be built once and reused.

M2: McpToolIndex.save() is not atomic (mcp_tool_index.rs:187)

std::fs::write is not atomic. Crash mid-write corrupts the index. Use write-to-temp-then-rename.

M3: McpToolIndex serializes index_path into the JSON file (mcp_tool_index.rs:43-47)

Loading from a different path than where it was saved sets self.index_path to the original save location. Subsequent save() writes to the wrong path. Mark index_path as #[serde(skip)] and set from the load argument.

M4: No input validation or size limits on deserialized data (procedure.rs:197, mcp_tool_index.rs:215)

Both deserialize arbitrary JSON from disk with no size limits. Learning system ingests data from failed command output which could contain attacker-controlled content. Add max file size check and field length validation.

Minor

• default_path fallback uses PathBuf::from("~/.config") -- tilde is not expanded by PathBuf, creates a literal ~ directory
• No dedup check in McpToolIndex::add_tool -- repeated discovery cycles bloat the index
• ProcedureConfidence fields are all pub -- score can be set inconsistently, bypassing recalculate_score() invariant
• save_with_dedup uses substring containment (Aho-Corasick) not semantic similarity -- "Rust setup" won't match existing "Install Rust"
• Latency benchmark assertion (50ms) will flake in slow CI environments

Missing

• procedure module is test-gated so ProcedureStore has zero production integration
• No size/growth management -- JSONL files grow without bound
• No concurrency tests despite file-based storage with potential concurrent writers

Documentation Preview

Your documentation changes have been deployed to:
https://0f5db506.terraphim-docs.pages.dev

This preview will be available until the PR is closed.